Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to safely inject delayed formatters. #1

Merged
2 commits merged into from
Nov 10, 2020
Merged

Conversation

Drup
Copy link
Contributor

@Drup Drup commented Mar 25, 2020

From the docs:

If you are familiar with the fmt library, Pp.to_fmt basically allows you to go from a 'a Pp.t to a 'a Fmt.t . The opposite is not possible; it is not possible to inject arbitrary side-effecting formatting functions into a Pp.t .

I find that a big deal breaker, as it makes the library incompatible with pp functions provided by most libraries (which is ironic, given the name). It also means it's just a formatter library for error messages, not a modular pretty printing library.

Fortunately, it's not that difficult to fix using the (moderatly recent) Format.kdprintf function, which was introduced precisely to write and transport error messages in the compiler. :)

A change I didn't do was to replace textf. textf is, in my opinon, a broken function which provides wrong expectation (tags and formatting are ignored, formatting decisions are taken too eagerly, etc) that should be replaced by the new Pp.pf.

Copy link
Contributor

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the change itself, but making 4.08 the minimum for this library seems a bit painful. @jeremiedimino wdyt?

@ghost
Copy link

ghost commented Jun 9, 2020

Sorry, I missed this PR initially. This restriction was intentional.

@Drup if you want easy interop with Format.t, why not use Fmt? If we had this function the API, I feel like there is not much reason for Pp to exist, it becomes pretty much the same as Fmt but with a more complicated implementation.

@rgrinberg
Copy link
Contributor

rgrinberg commented Jun 9, 2020 via email

@Drup
Copy link
Contributor Author

Drup commented Jun 10, 2020

The library aims to provide an easy to use interface for formatting messages. The new function doesn't compromise that, it just allows users to reuse existing pp functions, as @rgrinberg points out.

Personally I'm too far gone when it comes to format, so I use Fmt :). But I want users to be able to leverage the pretty printer I provide in my libraries.

Also, the fact that textf is so broken (I mean, we have been there before, the Lwt_io.*print* behave the same, and they have caused lot's of issues) is an argument by itself that you should have something better.

@ghost
Copy link

ghost commented Jun 10, 2020

I agree, but then it does open the question of the purpose of Pp and whether it is relevant at all. What value does it add to the world if Pp.t is the morally the same as Fmt.t?

FTR, the main motivation for writing Pp was to provide an API that forbids Format style format strings. Before Pp we were using Format style format strings for error messages in Dune, and they were pretty much all wrong. It was a combination of:

  • "a long text", which resulted in long unbreakable lines
  • "a\nlong\ntext", which basically bypasses the Format algorithm and results in frozen formatting that doesn't adapt to the output width
  • "%a" Format.pp_print_text "a long text", which formats correctly but is quite ugly
  • "a@ long@ text", which is about the same as the previous one

As a result, the formatting of messages printed by Dune was very inconsistent and not great.

@mshinwell also mentioned to me that such format strings can be difficult to work with, in the context of the compiler.

Overall, my impression is that writing good format strings requires expert knowledge. So I considered the following options:

  • do nothing
  • asks all the Dune devs to learn and become fluent in format strings, and carefully review format strings
  • write an API that doesn't allow the user of format strings, and instead forces to use combinators, which is something developers are more used to

I opted for the last option.

@Drup
Copy link
Contributor Author

Drup commented Jun 12, 2020

It seems to me your argument mainly concerns pf, but you are not opposed to of_fmt, which is sort-of an escape hatch for composition. Is that correct ?

As far as I'm concerned, of_fmt is the important bit (and the one @rgrinberg seems to like). We can remove the rest if you prefer, which also solves the ocaml version issue. Although in that case, It would be logical to remove textf as well.

@Drup
Copy link
Contributor Author

Drup commented Jun 12, 2020

Also, if you are interested, I'm pretty sure I can write a function text_printf which takes a format string, but interpret spaces as @ .

@ghost
Copy link

ghost commented Jun 12, 2020

It seems to me your argument mainly concerns pf, but you are not opposed to of_fmt, which is sort-of an escape hatch for composition. Is that correct ?

That seems like a good compromise, yes.

As far as I'm concerned, of_fmt is the important bit (and the one @rgrinberg seems to like). We can remove the rest if you prefer, which also solves the ocaml version issue. Although in that case, It would be logical to remove textf as well.

Well, textf has a clear semantic: it's text + sprintf. Both are are simple and well understood. If you look at the API without thinking of the Format module, it seems to me that this function makes sense.

Also, if you are interested, I'm pretty sure I can write a function text_printf which takes a format string, but interpret spaces as @ .

That would be nice. BTW, I feel like it would be simpler if Format's style format string were presented as just a "markdown format". Then a function like Format.fprintf would just be the combination of classic printf + this markdown. For instance:

(** [markdown s] interprets [s] as a string following format: ... *)
val markdown : string -> _ Pp.t

val markdownf : ('a, unit, 'b Pp.t) format -> 'a

@Drup
Copy link
Contributor Author

Drup commented Jun 15, 2020

Ok, I'll simplify the patch when I have time.

Well, textf has a clear semantic: it's text + sprintf. Both are are simple and well understood.

The problem is that sprintf has, by design, broken composition. You use its result for further pretty printing, which is precisely where it doesn't work well: the layout specifier won't work well, some layout decisions will be taken to eagerly, and tag don't work. Practice shows that the using sprintf for composition mostly results in broken formatting (and unreasonable numbers of string allocations).

sprintf should really only be used as a quick&dirty way of making your own formatter.

I feel like it would be simpler if Format's style format string were presented as just a "markdown format".

So you want to decouple the composition aspect from the layout aspect. In spirit, this is actually what Format.dprintf does (except with a closure for various side-effecting reasons). It is sometimes appropriate internally. However, your type for markdownf prevents proper use of %a/%t, which prevents good composition. From a semantic point of view, formatter -> unit (aka, %t) serves the same purpose as your Pp.t: an independent document for which precise layout has not been decided yet.

My impression is that %a/%t is not a difficult part of Format (and from a community perspective, it's clearly a win). The weird box model and the existing cruft is the problem.

@ghost
Copy link

ghost commented Jun 15, 2020

The problem is that sprintf has, by design, broken composition. You use its result for further pretty printing, which is precisely where it doesn't work well: the layout specifier won't work well, some layout decisions will be taken to eagerly, and tag don't work. Practice shows that the using sprintf for composition mostly results in broken formatting (and unreasonable numbers of string allocations).

Just to clarify: are you thinking of Format.sprintf or Printf.sprintf? I am thinking of Printf.sprintf, which doesn't know about layout specifiers or tags. It seems to me that the semantic of text + Printf.sprintf is pretty clear: you create a plain string and then convert spaces to Pp.space and newlines to Pp.newline.

I see Pp.textf as a function to create leaves of the document rather than internal nodes, and so there is not much composition involved at this level. When using the Pp module, one should open Pp.O and perform composition via the ++ operator.

So you want to decouple the composition aspect from the layout aspect. In spirit, this is actually what Format.dprintf does (except with a closure for various side-effecting reasons). It is sometimes appropriate internally. However, your type for markdownf prevents proper use of %a/%t, which prevents good composition. From a semantic point of view, formatter -> unit (aka, %t) serves the same purpose as your Pp.t: an independent document for which precise layout has not been decided yet.

Why does markdownf prevent proper use of %a/%t? My expectation is that you'd write Pp.markdownf "@[%a@]" f x where f would have type 'a -> 'b Pp.t.

My impression is that %a/%t is not a difficult part of Format (and from a community perspective, it's clearly a win). The weird box model and the existing cruft is the problem.

It's also the DSL you have to learn just for printing, i.e. @[, @ and so on. If you use Format you have to choose between a DSL and imperative functions. Pp replaces both of these by a more classic API with combinators.

Regarding the win from a community perspective, are you thinking of debugging functions that take a format and might print nothing?

@rgrinberg
Copy link
Contributor

It would be nice to get this in since I'd like to use pp in another project. @Drup if you don't have time, I can reduce your pr to just adding of_fmt, which is indeed just the bit that I need.

@Drup
Copy link
Contributor Author

Drup commented Nov 9, 2020

Ah, yes, I forgot about this. Please feel free to take/re-propose the bits you want. :)

@rgrinberg
Copy link
Contributor

@jeremiedimino should be ready now

Signed-off-by: Jeremie Dimino <[email protected]>
@ghost ghost merged commit 5cfc5f6 into ocaml-dune:master Nov 10, 2020
@ghost
Copy link

ghost commented Nov 10, 2020

Looks good, I added a disclaimer and merged. Thanks!

@mbarbin
Copy link
Collaborator

mbarbin commented Sep 13, 2024

Hello @Drup

I am working on preparing a release for the pp project - this will contain #17 which reverts some changes from this PR. This is motivated by #9 and the ability to serialize pps without running the risk of encountering dynamic failures.

I didn't spot any use for Pp.of_fmt using sherlocode. Do you have a use for it in a production path somewhere? I wanted to give you advance notice, just in case. Thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants